Skip to content

chore(backend): Limit exposed token verification related errors#2189

Merged
tmilewski merged 1 commit into
mainfrom
chore/limit-token-error-exports
Dec 7, 2023
Merged

chore(backend): Limit exposed token verification related errors#2189
tmilewski merged 1 commit into
mainfrom
chore/limit-token-error-exports

Conversation

@tmilewski

Copy link
Copy Markdown
Member

Description

Removes both TokenVerificationErrorAction & TokenVerificationErrorCode from @clerk/backend exports as both are internal.

It also happens to support @flawnn's work/use-case in #1980, once we backport to release/v4.

Checklist

  • npm test runs as expected.
  • npm run build runs as expected.
  • (If applicable) JSDoc comments have been added or updated for any package exports
  • (If applicable) Documentation has been updated

Type of change

  • 🐛 Bug fix
  • 🌟 New feature
  • 🔨 Breaking change
  • 📖 Refactoring / dependency upgrade / documentation
  • other:

Packages affected

  • @clerk/backend
  • @clerk/chrome-extension
  • @clerk/clerk-js
  • @clerk/clerk-expo
  • @clerk/fastify
  • gatsby-plugin-clerk
  • @clerk/localizations
  • @clerk/nextjs
  • @clerk/clerk-react
  • @clerk/remix
  • @clerk/clerk-sdk-node
  • @clerk/shared
  • @clerk/themes
  • @clerk/types
  • build/tooling/chore

Co-authored-by: flawnn <fl4wn@protonmail.com>
@tmilewski tmilewski self-assigned this Nov 22, 2023
@changeset-bot

changeset-bot Bot commented Nov 22, 2023

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 5410436

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages
Name Type
@clerk/clerk-sdk-node Major
@clerk/backend Major
@clerk/nextjs Major
@clerk/remix Major
gatsby-plugin-clerk Patch
@clerk/fastify Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@dimkl dimkl left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ How is dropping those 2 exports going to resolve the use case of user of catching and handling those errors? I think i got confused. 😓

@tmilewski

Copy link
Copy Markdown
Member Author

@dimkl It doesn't. A comment in the other PR suggested we wanted to keep some of these exports internal. This PR aims to narrow the scope of the exports on main. When backporting this to release/v4, it will enable access only to these narrowed exports which aren't currently exported at all.

@dimkl

dimkl commented Nov 23, 2023

Copy link
Copy Markdown
Contributor

I would suggest we close this PR because in the context of the Public APIs RFC we are going to change the exports of the @clerk/backend and probably export a @clerk/backend/errors subpath with the errors that we want to expose.
Also, I would suggest we keep those errors exported (from the /errors subpath) since the customer use case seems valid.

export { AuthStatus } from './authStatus';
export type { RequestState } from './authStatus';
export * from './errors';
export { TokenVerificationError, TokenVerificationErrorReason } from './errors';

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tmilewski As the discussion about the error-related exports for v5 is still ongoing, I think we need to do the following:

  • export the requested error from release/v4
  • remove export * from './errors' from here
  • revisit this then we make a decision

@tmilewski tmilewski added this pull request to the merge queue Dec 7, 2023
Merged via the queue into main with commit e1f7eae Dec 7, 2023
@tmilewski tmilewski deleted the chore/limit-token-error-exports branch December 7, 2023 15:10
octoper pushed a commit that referenced this pull request Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants